-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: Allow Point_Get during DDL with Global Index #56382
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @mjonss. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56382 +/- ##
================================================
+ Coverage 73.3645% 76.0517% +2.6871%
================================================
Files 1624 1646 +22
Lines 448069 456163 +8094
================================================
+ Hits 328724 346920 +18196
+ Misses 99207 87633 -11574
- Partials 20138 21610 +1472
Flags with carried forward coverage won't be shown. Click here to find out more.
|
// session should not see (may be duplicate errors on insert/update though) | ||
// For example during truncate or drop partition. | ||
func (pi *PartitionInfo) IDsInDDLToIgnore() []int64 { | ||
// TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple PRs/enhancements that will build upon this, especially for DROP and TRUNCATE partition.
/retest |
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Splitted this part from #55819 since it can be reviewed separately. |
pkg/ddl/partition.go
Outdated
ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, originalState != job.SchemaState) | ||
tblInfo.Partition.DDLState = job.SchemaState | ||
tblInfo.Partition.DDLAction = job.Type | ||
ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep originalState != job.SchemaState
. Maybe we could ask DDL member to confirm it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will always be true, since originalState
is set to job.SchemaState
before the switch, and is changed in each of the switch cases before the call. And it should update the Schema Version for each of the state changes.
if canConvertPointGet { | ||
if path != nil && path.Index != nil && path.Index.Global { | ||
// Don't convert to point get during ddl | ||
// TODO: Revisit truncate partition and global index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why point get didn't work before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it may have used partitions that should not have been seen, when it comes to global index, since the global index is unique it can be used for Point Get, and during DDL there might be old entries in the Global Index pointing to dropped partitions or new entries pointing to partitions being added or replacing old ones that are not yet seen by all sessions. Meaning a full table scan may see different data from a Global Index read.
args = append(args, expression.NewInt64Const(-1)) | ||
} else { | ||
// `PartitionPruning`` func does not return adding and dropping partitions | ||
// TODO: When PartitionPruning is guaranteed to not | ||
// return old/blocked partition ids then ignoreMap can be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it will happen? If the answer is yes, please add a related test case for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think idxArr
will not contained any paritition in AddingDefinitions
and DroppingDefinitions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are probably right, but I prefer to have all partitioning DDLs checked and tested for Global Index consistency and proper visibility for each state combination. So until then I prefer to have this check here. Or do you prefer to add an intest-assert in PartitionPruning instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an intest-assert is better for me.
} | ||
} | ||
if len(args) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it only happens in the first if-branch, maybe we could move into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one truncates all partitions or partition pruning don't find any matching partition, it could also happen in last if-branch.
tblID = pid | ||
if !matchPartitionNames(tblID, e.partitionNames, e.tblInfo.GetPartitionInfo()) { | ||
pi := e.tblInfo.GetPartitionInfo() | ||
if !matchPartitionNames(tblID, e.partitionNames, pi) { | ||
return nil | ||
} | ||
for _, id := range pi.IDsInDDLToIgnore() { | ||
if id == pid { | ||
return nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to converge all partition logic together into the PartitionProcessor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What problem does this PR solve?
Issue Number: ref #55819 , ref #45133
Problem Summary:
During DDL that affects Global Index, the PointGet access method is blocked, which may cause temporary bad query plans during partitioning management commands, like DROP/TRUNCATE/REORGANIZE PARTITION.
What changed and how does it work?
Allow PointGet, by filter away the partitions that should not be seen (like when the cleanup of the global index is ongoing).
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.